Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Misc cleanup #1770

Merged
merged 4 commits into from
Jun 4, 2024
Merged

Misc cleanup #1770

merged 4 commits into from
Jun 4, 2024

Conversation

fingolfin
Copy link
Member

  • Fix typo
  • Move some ZZMatrix code to fmpz_mat.jl
  • Enable 'f(x)' support where f is an arb/acb/real/complex poly
  • Remove redundant is_zero_row methods

For further details please refer to the respective commit messages.

@codecov Codecov
Copy link

codecov bot commented Jun 3, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 52.22222% with 43 lines in your changes missing coverage. Please review.

Project coverage is 85.59%. Comparing base (2c69d79) to head (0d0b53e).

❗ Current head 0d0b53e differs from pull request most recent head 5887a5a

Please upload reports for the commit 5887a5a to get more accurate results.

Files Patch % Lines
src/flint/fmpz_mat.jl 52.22% 43 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1770      +/-   ##
==========================================
- Coverage   85.80%   85.59%   -0.21%     
==========================================
  Files          95       95              
  Lines       36870    36905      +35     
==========================================
- Hits        31636    31590      -46     
- Misses       5234     5315      +81

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

fingolfin
fingolfin commented Jun 4, 2024
View reviewed changes
src/flint/fmpz_mat.jl

Reduces every entry modulo $p$, i.e. applies the mod function to every entry.
"""
function mod(M::ZZMatrix, p::ZZRingElem)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I note that there is an existing function reduce_mod which seems to do the same, albeit with a direct call to function fmpz_mat_scalar_mod_fmpz which I am guessing be more efficient?

What should we do, keep both reduce_mod and mod? Or merge them? Thoughts, @fieker @thofma ?

@fingolfin fingolfin requested a review from lgoettgens June 4, 2024 07:35
lgoettgens
lgoettgens approved these changes Jun 4, 2024
View reviewed changes
Copy link
Collaborator

@lgoettgens lgoettgens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apart from the one unresolved question, this looks good

@lgoettgens
Copy link
Collaborator

Unfortunately we have conflicts with #1768

@fingolfin
Fix typo
d511f1a
@fingolfin
Move some ZZMatrix code to fmpz_mat.jl
e9293e4
@fingolfin
Enable 'f(x)' support where f is an arb/acb/real/complex poly
5451795
@fingolfin
Remove redundant is_zero_row methods
5887a5a 
The generic ones in AbstractAlgebra already takes care of these,
and are equally efficient thanks to is_zero_entry
@fingolfin
Copy link
Member Author

rebased. And I've filed #1773 for reduce_mod vs. mod, we don't need to settle it in this PR -- at least now the code for both is next to each other, making the duplication a bit more "obvious"

@lgoettgens
Copy link
Collaborator

rebased. And I've filed #1773 for reduce_mod vs. mod, we don't need to settle it in this PR -- at least now the code for both is next to each other, making the duplication a bit more "obvious"

Thanks for both!

@lgoettgens lgoettgens enabled auto-merge (squash) June 4, 2024 12:42
@lgoettgens lgoettgens merged commit c3368ac into Nemocas:master Jun 4, 2024
24 checks passed
@fingolfin fingolfin deleted the mh/cleanup branch June 4, 2024 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lgoettgens lgoettgens lgoettgens approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fingolfin @lgoettgens